-
Notifications
You must be signed in to change notification settings - Fork 13.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add connector for CouchbaseDB #29225
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #29225 +/- ##
==========================================
+ Coverage 60.48% 70.34% +9.85%
==========================================
Files 1931 1970 +39
Lines 76236 78585 +2349
Branches 8568 8973 +405
==========================================
+ Hits 46114 55279 +9165
+ Misses 28017 21108 -6909
- Partials 2105 2198 +93
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Any | ||
) | ||
|
||
class CouchbaseEngineSpec(BaseEngineSpec): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can you add BasicParametersMixin
to this since using MYSQL dialect
https://github.com/apache/superset/blob/master/superset/db_engine_specs/base.py#L2276
Thanks @ayush33143314 for adding the engine. Would you also mind:
|
Thank you for this! Would it also be possible to add a docs entry here? |
Hi , yes, I am preparing the doc, I will submit doc along with tests in next patchset. Sorry for this patch. It is still in progress. |
Ahh, ok, thanks! Feel free to add [WIP] to the title and/or convert it to a draft PR if you need a while to keep working on it, just so others don't assume it's ready for review like I did. Thanks again! |
@hughhhh @john-bodley Please review my latest patch, and guide me if I need to make any change. Thank you |
Looking forward to this PR being merged |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks mostly good, but added a few comments/questions.
@classmethod | ||
def epoch_to_dttm(cls) -> str: | ||
return "MILLIS_TO_STR({col} * 1000, '111')" | ||
|
||
@classmethod | ||
def epoch_ms_to_dttm(cls) -> str: | ||
return "MILLIS_TO_STR({col}, '111')" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried reading up on the docs, and I didn't fully understand how this format is valid:
MILLIS_TO_STR
: https://docs.couchbase.com/cloud/n1ql/n1ql-language-reference/datefun.html#fn-date-millis-to-str- Date format: https://docs.couchbase.com/cloud/n1ql/n1ql-language-reference/datefun.html#date-string
To be clear, the idea of these methods is to provide an expression that can convert an epoch (seconds since 1970-01-01 00:00:00 in UTC) into a native temporal format. But if it works it works - if this is the case, maybe adding a comment as to why the format '111'
works here would be helpful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much for your comments,
If format is unspecified or incorrect it gives combined date and time in result , On querying
SELECT MILLIS_TO_STR(1463284740000,'111'); it gives me "2016-05-15T03:59:00Z" in result . I had this '111' just for notation . But I have removed this in my new patch , because MILLIS_TO_STR(1463284740000) also gives me same results.
("Date", "DATETIME(DATE_FORMAT_STR(STR_TO_UTC('2019-01-02'), 'iso8601'))"), | ||
( | ||
"DateTime", | ||
"DATETIME(DATE_FORMAT_STR(STR_TO_UTC('2019-01-02T03:04:05'), 'iso8601'))", | ||
), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to have a few more typical formats featured here, like VARCHAR
, TEXT
, INT
, as I assume those will be commonly used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added test case for other data types also.
0a11aff
to
d2f3da0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One small last comment, we can probably do improvements in follow-up PRs. Also, since CounchbaseDB doesn't seem to have any dedicated temporal types, have you been able to get it to work fully with temporal charts? If not, you may benefit from specifying column_type_mutators
on your spec to handle the conversion after query results come in.
Co-authored-by: ayush-couchbase <[email protected]>
SUMMARY
Add support for couchbase db. https://www.couchbase.com/products/analytics/
TESTING INSTRUCTIONS
mannual Testing
ADDITIONAL INFORMATION